Increase static typing strictness #10530

AA-Turner · 2022-06-05T18:37:46Z

Goes from 916 to 650 errors with strict_optional = True.

Feature or Bugfix

Bugfix
Refactoring

xref: #10528

A

AA-Turner · 2022-06-05T18:37:55Z

cc: @danieleades

A

sphinx/util/parallel.py

sphinx/registry.py

sphinx/application.py

danieleades

there's a few blanket warnings suppressions that can be tightened, although these are technically adjacent to the changed code

sphinx/application.py

sphinx/builders/html/__init__.py

danieleades · 2022-06-06T15:44:24Z

sphinx/builders/__init__.py

    supported_data_uri_images = False

-    def __init__(self, app: "Sphinx") -> None:
+    def __init__(self, app: "Sphinx", env: BuildEnvironment = None) -> None:


Suggested change

def __init__(self, app: "Sphinx", env: BuildEnvironment = None) -> None:

def __init__(self, app: "Sphinx", env: Optional[BuildEnvironment] = None) -> None:

Here I'm using None as a sentinel, I could have equally used .... I don't want to indicate to users that None is a valid type, which I believe using Optional[] syntax would do. Hence I'm -1 on both these suggestions, unless you've alternate ideas?

A

that's not how this works. Setting a default value of None effectively coerces the type from Type to Optional[Type]. That is, the former desugars to the latter implicitly. The setting that controls this is called no-implicit-optional.

It's better to be explicit and use the Optional. If using 'strict' mypy checking, failing to use it becomes a hard error.

I added no_implicit_optional = True and everything still passes -- is this what you meant?

A

Yep, it should be an error with that config. Not sure why you're not seeing one, could be you're hitting the maximum number of errors, or it could be a false negative. In either case you should be able to convince yourself from the documentation that using the explicit Optional is strictly correct.

ah i think i see the issue. you have this-

strict_optional = False no_implicit_optional = True

to see these errors your would need

strict_optional = True no_implicit_optional = True

if you really want to give yourself nightmares, try

strict = True

danieleades · 2022-06-06T15:45:04Z

sphinx/builders/html/__init__.py


-    def __init__(self, app: Sphinx) -> None:
-        super().__init__(app)
+    def __init__(self, app: Sphinx, env: BuildEnvironment = None) -> None:


Suggested change

def __init__(self, app: Sphinx, env: BuildEnvironment = None) -> None:

def __init__(self, app: Sphinx, env: Optional[BuildEnvironment] = None) -> None:

tk0miya · 2022-06-06T17:40:29Z

sphinx/registry.py

+                f"'env'argument. Report this bug to the developers of your custom builder, "
+                f"this is likely not a issue with Sphinx. The 'env' argument will be required "
+                f"from Sphinx 7.", RemovedInSphinx70Warning, stacklevel=2)
+            return self.builders[name](app)


The new code never calls set_environment(). As a result, custom builders will get crashed because the env object is None.

This will also cause another warning on Builder.__init__(). The users will see warnings twice or more.

Please be careful to changing the API. I believe Sphinx is not only an application but also a framework. So I consider this is a mix of refactoring and API changing. Is it intended?

I was concerned the potential to call set_environment twice, but it seems that wouldn't be a problem, so updated. I also added a check so that we only issue one warning.

Please be careful to changing the API. I believe Sphinx is not only an application but also a framework. So I consider this is a mix of refactoring and API changing. Is it intended?

This PR shouldn't have any end-user facing changes. I would say I consider Sphinx more of an application with a plugin mechanism though, rather than a library/framework -- we should be able to make changes that improve things on our end, although of course seeking to keep backwards compatability.

A

danieleades · 2022-06-06T20:36:49Z

@AA-Turner this is already a herculean effort, nice work!

If you're not able to squash all of the errors in this PR, you may want to consider whitelisting a subset of files with strict_optional = False, so that the CI checks can run on the rest. Then the whitelist can be incrementally shrunk in subsequent PRs. See my comment over here - #10528 (comment)

that would look something like

current:

[mypy]
python_version = 3.6
disallow_incomplete_defs = True
show_column_numbers = True
show_error_context = True
ignore_missing_imports = True
follow_imports = skip
check_untyped_defs = True
warn_unused_ignores = True
strict_optional = False

new:

[mypy]
python_version = 3.6
disallow_incomplete_defs = True
show_column_numbers = True
show_error_context = True
ignore_missing_imports = True
follow_imports = skip
check_untyped_defs = True
warn_unused_ignores = True

[mypy-some.module]
strict_optional = False

[mypy-some.other.module]
strict_optional = False

(i think? i usually use pyproject.toml for configuring mypy)

AA-Turner · 2022-06-06T22:29:37Z

I'm not going to do the whitelist approach -- I got to 34 modules and the list is ridiculous (100+ lines long) and there are still 244 errors reported.

My progress is in the details block for posterity, but I don't reccomend adding it to setup.cfg.

Details

strict_optional = True
no_implicit_optional = True

[mypy-sphinx.application]
strict_optional = False

[mypy-sphinx.writers.latex]
strict_optional = False

[mypy-sphinx.builders.latex]
strict_optional = False

[mypy-sphinx.builders.html]
strict_optional = False

[mypy-sphinx.builders.gettext]
strict_optional = False

[mypy-sphinx.builders.singlehtml]
strict_optional = False

[mypy-sphinx.builders.dirhtml]
strict_optional = False

[mypy-sphinx.builders.latex.transforms]
strict_optional = False

[mypy-sphinx.domains.c]
strict_optional = False

[mypy-sphinx.domains.cpp]
strict_optional = False

[mypy-sphinx.domains.javascript]
strict_optional = False

[mypy-sphinx.domains.python]
strict_optional = False

[mypy-sphinx.domains.changeset]
strict_optional = False

[mypy-sphinx.builders.xml]
strict_optional = False

[mypy-sphinx.builders.manpage]
strict_optional = False

[mypy-sphinx.builders.texinfo]
strict_optional = False

[mypy-sphinx.writers.texinfo]
strict_optional = False

[mypy-sphinx.writers.text]
strict_optional = False

[mypy-sphinx.writers.html5]
strict_optional = False

[mypy-sphinx.writers.html]
strict_optional = False

[mypy-sphinx.environment]
strict_optional = False

[mypy-sphinx.environment.adapters.indexentries]
strict_optional = False

[mypy-sphinx.util.docfields]
strict_optional = False

[mypy-sphinx.directives.code]
strict_optional = False

[mypy-sphinx.transforms.post_transforms.images]
strict_optional = False

[mypy-sphinx.builders.linkcheck]
strict_optional = False

[mypy-sphinx.ext.autosummary]
strict_optional = False

[mypy-sphinx.ext.autodoc]
strict_optional = False

[mypy-sphinx.ext.apidoc]
strict_optional = False

[mypy-sphinx.ext.autosummary.generate]
strict_optional = False

[mypy-sphinx.ext.imgmath]
strict_optional = False

[mypy-sphinx.ext.intersphinx]
strict_optional = False

[mypy-sphinx.ext.mathjax]
strict_optional = False

[mypy-sphinx.ext.graphviz]
strict_optional = False

[mypy-sphinx.ext.inheritance_diagram]
strict_optional = False

A

AA-Turner · 2022-06-06T22:31:52Z

Followup:

If you're not able to squash all of the errors in this PR

This PR was never about getting all of them, it started as ensuring MyPy passes on Windows and then I fixed some "easy" targets. C.f. Zen, pragmatism & purity...!

A

AA-Turner added 6 commits June 5, 2022 18:05

Assert readline setup isn't Windows for static typing

ae759f4

Add Windows check to get_terminal_width

f65241a

Define ForkProcess on Windows

3449e9b

Fix default type for getattr calls

8834ab7

Make Sphinx.(builder|env|project) non None

e98bac9

Make Builder.env non None

6616779

AA-Turner added the internals:refactoring label Jun 5, 2022

AA-Turner added this to the 5.x milestone Jun 5, 2022

tk0miya requested changes Jun 6, 2022

View reviewed changes

sphinx/util/parallel.py Outdated Show resolved Hide resolved

sphinx/registry.py Outdated Show resolved Hide resolved

sphinx/application.py Outdated Show resolved Hide resolved

sphinx/application.py Show resolved Hide resolved

AA-Turner added 4 commits June 6, 2022 16:02

Use ForkProcess without quoting

95105f1

Backwards compat for create_builder

d62b576

Move _fresh_env_used to the Sphinx object.

59dc8d1

Line length

35542cd

danieleades requested changes Jun 6, 2022

View reviewed changes

sphinx/application.py Outdated Show resolved Hide resolved

sphinx/application.py Outdated Show resolved Hide resolved

sphinx/builders/html/__init__.py Outdated Show resolved Hide resolved

danieleades reviewed Jun 6, 2022

View reviewed changes

AA-Turner added 6 commits June 6, 2022 16:59

Use a runtime type alias

c373bd0

Show MyPy's error codes

08f0a42

Specific ignore

c2eedd7

Line length

0bf4d55

It seems the ignore must be exactly on the line of the function call...

7377de1

Update create_builder to handle custom builders

b4a92ac

tk0miya reviewed Jun 6, 2022

View reviewed changes

AA-Turner added 3 commits June 6, 2022 18:50

Add builder.set_environment calls

c1ed2fb

Ignore intentional type breakage

63a803e

Add no_implicit_optional config setting

4657261

AA-Turner changed the title ~~Fix static typing issues on Windows; increase Optional strictness~~ Increase static typing strictness Jun 16, 2022

AA-Turner merged commit 6ef22d2 into sphinx-doc:5.x Jun 16, 2022

AA-Turner deleted the typing-strictness branch June 16, 2022 18:50

AA-Turner modified the milestones: 5.x, 5.1.0 Jun 16, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2022

	def __init__(self, app: "Sphinx", env: BuildEnvironment = None) -> None:
	def __init__(self, app: "Sphinx", env: Optional[BuildEnvironment] = None) -> None:

	def __init__(self, app: Sphinx, env: BuildEnvironment = None) -> None:
	def __init__(self, app: Sphinx, env: Optional[BuildEnvironment] = None) -> None:

Uh oh!

Increase static typing strictness #10530

Increase static typing strictness #10530

Uh oh!

Conversation

AA-Turner commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Feature or Bugfix

Uh oh!

AA-Turner commented Jun 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danieleades left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieleades commented Jun 6, 2022

Uh oh!

AA-Turner commented Jun 6, 2022

Uh oh!

AA-Turner commented Jun 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AA-Turner commented Jun 5, 2022 •

edited

Loading